Skip to content

Implement RFC 3503: frontmatters #140035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 6, 2025

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Apr 19, 2025

Tracking issue: #136889

Supercedes #137193. This implements RFC 3503.

This might break rust-analyzer. Will look into how to fix that.

Suggestions welcome for how to improve diagnostics.

@rustbot
Copy link
Collaborator

rustbot commented Apr 19, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Apr 19, 2025
@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu added the F-frontmatter `#![feature(frontmatter)]` label Apr 19, 2025
@jieyouxu
Copy link
Member

I'll do a review pass next Monday/Tuesday, but in the meantime, I'll roll this to Wesley who I think have more context on the general vibes.

r? @wesleywiser

@rustbot rustbot assigned wesleywiser and unassigned jieyouxu Apr 19, 2025
@jieyouxu jieyouxu self-assigned this Apr 19, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the implementation! I left some (adversarial) review notes under the impression based on reading back #137193 that frontmatter is a Rust language syntax. However, the existing diagnostics here I find is already quite nice.

I especially appreciate using error annotations in between frontmatter openers/closers, I found that very clever (and cute) 😆

Comment on lines +1 to +6
#![feature(frontmatter)]

---
//~^ ERROR: expected item, found `-`
// FIXME(frontmatter): make this diagnostic better
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark: yeah, I'm not sure how to detect this properly without making some convoluted logic, even though I imagine this would be one of the more common mistakes to make. At least this does error, maybe we could provide some kind of contextual HELP like if we see --- (3+ starting dashes)

error: expected item, found `-`
  --> $DIR/frontmatter-after-tokens.rs:3:1
   |
LL | ---
   | ^ expected item
   |
   = note: for a full list of items that can appear in modules, see <https://doc.rust-lang.org/reference/items.html>
   = help: if you meant to write a frontmatter, the frontmatter must come after an optional shebang but before any regular source code

(With some better wording, I find it tricky to explain)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Words are hard 🙂

If the file doesn't start with a shebang line, maybe we could just say something like

   = help: if you meant to write a frontmatter, the frontmatter must appear first in the file

Shebangs are not super commonly used so if we can leave that detail out, it would simplify the error message.

Copy link
Member Author

@fee1-dead fee1-dead Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this can be achieved not at the lexer/parser level but at the tokens to AST level. That code would not reside in places this PR touches so we can do this as a separate followup! :)

@bors
Copy link
Collaborator

bors commented Apr 23, 2025

☔ The latest upstream changes (presumably #140180) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines +1 to +6
#![feature(frontmatter)]

---
//~^ ERROR: expected item, found `-`
// FIXME(frontmatter): make this diagnostic better
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Words are hard 🙂

If the file doesn't start with a shebang line, maybe we could just say something like

   = help: if you meant to write a frontmatter, the frontmatter must appear first in the file

Shebangs are not super commonly used so if we can leave that detail out, it would simplify the error message.

@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2025
@wesleywiser
Copy link
Member

cc @epage for awareness 🙂

@epage
Copy link
Contributor

epage commented Apr 30, 2025

Thanks for your work on this!

#[cfg(debug_assertions)]
prev: char,
}

pub(crate) const EOF_CHAR: char = '\0';

impl<'a> Cursor<'a> {
pub fn new(input: &'a str) -> Cursor<'a> {
pub fn new(input: &'a str, frontmatter_allowed: bool) -> Cursor<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have an implementation using the lexer, instead of stripping (#137193), what are your thoughts on the trade offs?

The cases I can think of

  • General code design. I can't speak to this
  • Other tools (r-a, rustfmt, etc). I suspect this will need slightly more work for them to passively support the new syntax. For richer features (processing the content, mutating), either they'll still need their own parser or the token's capabilities will need to be expanded which is where we were at with Add unstable frontmatter support #137193
  • Other uses for the token existing? I can't think of any but maybe you have some in mind.
  • Diagnostics: Provide helpful diagnostics for shebang lookalikes #137619 shows that stripping can still have decent diagnostics. I can't speak to how bad it would be to implement each side

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#137619 still does error reporting in the parsing step, which still suggests we can't report errors in the lexer level. As far as my knowledge of the compiler goes, parsing it as a token and reporting it when the token gets "cooked" in the parsing step is pretty much the best way. That is how the implementation for c"str" literals worked and it was stabilized with that implementation.

@epage
Copy link
Contributor

epage commented Apr 30, 2025

@ytmimi as this doesn't update rustfmt for frontmatter support (unsure if that is intended for this PR or a follow up), any feedback on how well this will work for rustfmt, at least in terms of tolerating them (ie feature parity with #137193)

@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2025

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@fmease
Copy link
Member

fmease commented May 1, 2025

Well, rustfmt ought not strip frontmatter but reproduce it verbatim similar to shebangs.

@jieyouxu
Copy link
Member

jieyouxu commented May 2, 2025

Re. permitting whitespace between dashes and infostring #140035 (comment).
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 2, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@fee1-dead fee1-dead force-pushed the push-oszwkkvmpkks branch from 70a2a0f to 818ec6b Compare May 3, 2025 03:47
@fee1-dead
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 3, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impl looks good to me, not much to say other than a few tiny test nits (that I realized just now).

@fee1-dead fee1-dead force-pushed the push-oszwkkvmpkks branch from 818ec6b to 7917bee Compare May 4, 2025 04:17
@jieyouxu
Copy link
Member

jieyouxu commented May 4, 2025

LGTM, waiting a bit in case Wesley has more feedback.

Comment on lines 581 to 583
.or_else(|| rest.find("\nuse"))
.or_else(|| rest.find("\n//!"))
.or_else(|| rest.find("\n#!["));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Rather than searching for use then //! then #![, it might be better to find the first occurrence of any of those. Might also be good to search for "\nuse " as you could imagine a frontmatter of Cargo.toml using a user_auth = 1.0 dep or something.

Not a big deal though and can certainly be changed later 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it to "\nuse " but left the second part of that. I've noted down the other part of this suggestion in my todo list, because I think it would require pulling in aho-corasick, which is probably more suitable as a followup.

@wesleywiser
Copy link
Member

Feel free to r=jieyouxu,wesleywiser with or without that nit!

@fee1-dead fee1-dead force-pushed the push-oszwkkvmpkks branch from 7917bee to 6621826 Compare May 5, 2025 15:10
@fee1-dead
Copy link
Member Author

@bors r=jieyouxu,wesleywiser

@bors
Copy link
Collaborator

bors commented May 5, 2025

📌 Commit 6621826 has been approved by jieyouxu,wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 5, 2025
…s, r=jieyouxu,wesleywiser

Implement RFC 3503: frontmatters

Tracking issue: rust-lang#136889

Supercedes rust-lang#137193. This implements [RFC 3503](https://github.com/rust-lang/rfcs/blob/master/text/3503-frontmatter.md).

This might break rust-analyzer. Will look into how to fix that.

Suggestions welcome for how to improve diagnostics.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2025
…llaumeGomez

Rollup of 11 pull requests

Successful merges:

 - rust-lang#139764 (Consistent trait bounds for ExtractIf Debug impls)
 - rust-lang#140035 (Implement RFC 3503: frontmatters)
 - rust-lang#140080 (mir-opt: Use one MirPatch in MatchBranchSimplification)
 - rust-lang#140115 (mir-opt: execute MatchBranchSimplification after GVN)
 - rust-lang#140357 (bypass linker configuration and cross target check on `x check`)
 - rust-lang#140374 (Resolve instance for SymFn in global/naked asm)
 - rust-lang#140393 (std: get rid of `sys_common::process`)
 - rust-lang#140532 (Fix RustAnalyzer discovery of rustc's `stable_mir` crate)
 - rust-lang#140559 (Removing rustc_type_ir in the rustc_infer codebase)
 - rust-lang#140636 (implement `PanicTracker` to track `t` panics)
 - rust-lang#140661 (Make `-Zfixed-x18` into a target modifier)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request May 6, 2025
…s, r=jieyouxu,wesleywiser

Implement RFC 3503: frontmatters

Tracking issue: rust-lang#136889

Supercedes rust-lang#137193. This implements [RFC 3503](https://github.com/rust-lang/rfcs/blob/master/text/3503-frontmatter.md).

This might break rust-analyzer. Will look into how to fix that.

Suggestions welcome for how to improve diagnostics.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2025
Rollup of 12 pull requests

Successful merges:

 - rust-lang#139550 (Fix `-Zremap-path-scope` rmeta handling)
 - rust-lang#139773 (Implement `Iterator::last` for `vec::IntoIter`)
 - rust-lang#140035 (Implement RFC 3503: frontmatters)
 - rust-lang#140176 (Fix linking statics on Arm64EC)
 - rust-lang#140251 (coverage-dump: Resolve global file IDs to filenames)
 - rust-lang#140393 (std: get rid of `sys_common::process`)
 - rust-lang#140532 (Fix RustAnalyzer discovery of rustc's `stable_mir` crate)
 - rust-lang#140598 (Steer docs to `utf8_chunks` and `Iterator::take`)
 - rust-lang#140634 (Use more accurate ELF flags on MIPS)
 - rust-lang#140673 (Clean rustdoc tests folder)
 - rust-lang#140678 (Be a bit more relaxed about not yet constrained infer vars in closure upvar analysis)
 - rust-lang#140687 (Update mdbook to 0.4.49)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2025
Rollup of 12 pull requests

Successful merges:

 - rust-lang#139550 (Fix `-Zremap-path-scope` rmeta handling)
 - rust-lang#139764 (Consistent trait bounds for ExtractIf Debug impls)
 - rust-lang#139773 (Implement `Iterator::last` for `vec::IntoIter`)
 - rust-lang#140035 (Implement RFC 3503: frontmatters)
 - rust-lang#140251 (coverage-dump: Resolve global file IDs to filenames)
 - rust-lang#140393 (std: get rid of `sys_common::process`)
 - rust-lang#140532 (Fix RustAnalyzer discovery of rustc's `stable_mir` crate)
 - rust-lang#140598 (Steer docs to `utf8_chunks` and `Iterator::take`)
 - rust-lang#140634 (Use more accurate ELF flags on MIPS)
 - rust-lang#140673 (Clean rustdoc tests folder)
 - rust-lang#140678 (Be a bit more relaxed about not yet constrained infer vars in closure upvar analysis)
 - rust-lang#140687 (Update mdbook to 0.4.49)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 627873a into rust-lang:master May 6, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-frontmatter `#![feature(frontmatter)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants